Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reusable blocks: Improve UX for non-privileged users #12378

Merged
merged 16 commits into from
Jan 23, 2019

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Nov 28, 2018

What this is

Fixes #12338.

Improves the UX of creating, editing, and deleting a reusable block when logged in as an author or contributor by disabling the Add to Reusable Blocks, Edit, and Remove from Reusable Blocks buttons when necessary.

This is accomplished under the hood by introducing the canUser() selector to core-data which allows callers to query whether the REST API supports performing a given action on a given resource, e.g. one can query whether the logged in user can create posts by running wp.data.select( 'core' ).canUser( 'create', 'posts' ).

The existing hasUploadPermissions() selector is changed to use canUser( 'create', 'media' ) under the hood.

How it looks

Add to Reusable Blocks button is visible for admins and editors:

admin - add

Add to Reusable Blocks button is hidden for contributors:

contributor - add

Edit button is enabled and Remove from Reusable Blocks is visible for admins and editors:

admin - edit remove

Edit button is disabled and Remove from Reusable Blocks is hidden for authors:

author - edit remove

How to test

  1. Try to create a reusable block as a contributor. The Add to Reusable Blocks button should be disabled.
  2. Try to updating another user's reusable block as an author. The Edit button should be disabled.
  3. Try to deleting another user's reusable block as an author or contributor. The Remove from Reusable Blocks button should be disabled.

@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Status] In Progress Tracking issues with work in progress [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Nov 28, 2018
@noisysocks noisysocks added this to the 4.7 milestone Nov 28, 2018
@mtias mtias modified the milestones: 4.7, 4.8 Nov 29, 2018
@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from a1856af to 9a4cb69 Compare December 5, 2018 08:10
@noisysocks noisysocks removed the [Status] In Progress Tracking issues with work in progress label Dec 5, 2018
@noisysocks
Copy link
Member Author

noisysocks commented Dec 5, 2018

This should be ready to review!

@mtias: Reckon we could plop this back into 4.7? I'd like to have this fixed for 5.0.1 since it's a pretty awful UX otherwise. Not sure where we are timing wise. edit: Eh, 5.0.2 is OK 🙂

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from 9a4cb69 to 3c1db96 Compare December 6, 2018 00:57
talldan
talldan previously requested changes Dec 6, 2018
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the canUser function, it's a nice way to express that functionality.

I think that flash of the disabled state of the block settings menu item is something that needs to be fixed.

Preloading could work, but I think checking the resolution state is also possible and worth doing as well, since it shouldn't just depend on preloading. I think this is what embeds do:
https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/selectors.js#L41

packages/core-data/src/selectors.js Show resolved Hide resolved
@@ -80,6 +83,7 @@ export default compose( [
! isReusableBlock( blocks[ 0 ] ) ||
! getReusableBlock( blocks[ 0 ].attributes.ref )
),
canCreateBlocks: canUser( 'create', 'blocks' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For media upload permissions the media options request is preloaded. I think it might be worth doing the same here.

I noticed a 'flash' where the menu item went from being enabled to disabled when opening the block settings menu while the blocks options request was in progress.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 I've added this OPTIONS request to the list of requests we preload in bc0709f.

Noting that this is a change we'll need to make to core after this PR is merged.

*
* @return {boolean} Whether or not the user can perform the action.
*/
export function canUser( state, action, resource, id ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the expressiveness of the canUser selector 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Props to @gziolo for suggesting the API 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you like it :)

return {
reusableBlock: block && isReusableBlock( block ) ? getReusableBlock( block.attributes.ref ) : null,
id,
isDisabled: !! id && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think same comment here regarding disabling vs. hiding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also noting that in testing I didn't see the flash of this switching from enabled to disabled. That seems to be because the GET request for the block has already been made and cached. If an OPTIONS request was made instead of a GET request the issue would resurface.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have changed this to remove the button instead of disabling in 708a50484fc99537a5f3368b4dbb66e134198741.

We can't preload these requests because we don't know the reusable block ID ahead of time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I do understand that we can't preload. My worry is that it's only by coincidence that the user's capability is known ahead of time. If the underlying code switches to use OPTIONS instead of GET, users would start seeing this button initially be clickable but then suddenly disappear.

This is probably a nitpick though and not a blocker. I'm not even sure what the best UX would be.

Copy link
Member Author

@noisysocks noisysocks Dec 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it! Not sure if defaulting to disabled versus defaulting to enabled would be a better experience. Both aren't ideal. I matched the existing hasUploadPermissions behaviour which defaults to enabled.

I think that, further down the track, core-data should modify the canUser state whenever it receives an Allow header from any response, and not just when the canUser() resolver is called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a nice idea. Not sure how it'd work, but a nice idea 😄

path,
// Ideally this would always be an OPTIONS request, but unfortunately there's
// a bug in the REST API which causes the Allow header to not be sent on
// OPTIONS requests to /posts/:id routes.
Copy link
Contributor

@talldan talldan Dec 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that just for /post/:id of for the all resources? I mentioned that because the logic below happens for all resources (so doesn't match the comment).

Is there a ticket/issue covering the bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only checked posts but I suspect the bug affects all REST endpoints. Will do some investigating and open up a Trac ticket 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the case, it should be fixed /cc @danielbachhuber @kadamwhite

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just want the Allow header, what about doing a HEAD request instead of GET.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this further:

  1. The Allow header is generated by rest_send_allow_header(), which is called on rest_post_dispatch.
  2. rest_post_dispatch is called for every response (I think):

It seems like Allow should be present for OPTIONS. It's an officially supported header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/OPTIONS

Can you provide steps to reproduce?

Copy link
Member Author

@noisysocks noisysocks Dec 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I briefly debugged this when I noticed the issue and from memory it was something to do with rest_handle_options_request not calling $request->set_url_params() unlike dispatch which correctly parses and sets URL parameters.

Can you provide steps to reproduce?

  1. Create a reusable block
  2. Perform a GET http://local.wordpress.test/wp-json/wp/v2/blocks request and note the ID of the reusable block
  3. Perform a OPTIONS http://local.wordpress.test/wp-json/wp/v2/blocks/27 request, replacing 27 with the ID from step 2. Note that an Allow header is not sent back
  4. Perform a GET http://local.wordpress.test/wp-json/wp/v2/blocks/27 request, replacing 27 with the ID from step 2. Note that an Allow header is sent back

Above steps also work with posts: just change blocks to posts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a link to the Trac ticket in cf2d701.

@@ -103,7 +103,46 @@ export function* getEmbedPreview( url ) {
* Requests Upload Permissions from the REST API.
*/
export function* hasUploadPermissions() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I mark this as deprecated? I'm unsure on what the policy is post 5.0 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either. Might be worth pinging someone on slack. Will hold off approving until the answer is known.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine leaving this as is where we support both canUser and hasUploadPermissions. We can always deprecate later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to use a soft deprecation by using @deprecated in JSDoc and maybe filter out all actions and selectors from the generated docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for now, we should leave those, we can add a deprecation message if needed but without specifying a version.

This is something we should discuss more in the next #core-js chats.

Copy link
Member Author

@noisysocks noisysocks Dec 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noisysocks I noticed you @deprecated the selector, but the resolver hasn't been deprecated. I guess technically they're the same interface and the resolver wouldn't be called directly, but I wanted to double-check if it was intentional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I think selectors are the public interface here but it wouldn't hurt to have the @deprecated attribute in the resolver as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4ea46a5!

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch 2 times, most recently from 708a504 to 9ec72e8 Compare December 7, 2018 03:53
@noisysocks
Copy link
Member Author

@gziolo any thoughts on this one?

@@ -103,7 +103,46 @@ export function* getEmbedPreview( url ) {
* Requests Upload Permissions from the REST API.
*/
export function* hasUploadPermissions() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to use a soft deprecation by using @deprecated in JSDoc and maybe filter out all actions and selectors from the generated docs.


const method = methods[ action ];
if ( ! method ) {
throw new Error( `'${ action }' is not a valid action` );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't consistent, but probably we should end all sentences with a period.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ccf4936

throw new Error( `'${ action }' is not a valid action` );
}

const path = id ? `/wp/v2/${ resource }/${ id }` : `/wp/v2/${ resource }`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const path = id ? `/wp/v2/${ resource }/${ id }` : `/wp/v2/${ resource }`;
const path = '/wp/v2/${ resource }' + ( id ? `/${ id }` : '' );

would be shorter, not sure what reads better though :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that with the more verbose version that you can glance at the code and see the "shape" of the URL.

@@ -116,5 +155,7 @@ export function* hasUploadPermissions() {
allowHeader = get( response, [ 'headers', 'Allow' ], '' );
}

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice trick :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd need template strings for the first part:

Suggested change
const key = compact( [ action, resource, id ] ).join( '/' );
const path = `/wp/v2/${ resource }` + ( id ? `/${ id }` : '' );

I think it's a bit easier to sort out though, yeah.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this suggestion meant to go in this thread? #12378 (comment)

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );
const isAllowed = includes( allowHeader, method );
yield receiveUserPermissions( key, isAllowed );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be named receiveUserPermission using singular version to reflect that you can provide only one permission at the time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least could we add the ticket URL in the comment so it can be tracked/checked on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be named receiveUserPermission using singular version to reflect that you can provide only one permission at the time?

👍 02d9502

At the very least could we add the ticket URL in the comment so it can be tracked/checked on?

I think this is referring to this thread? #12378 (comment) If so I added a link to the Trac ticket in
cf2d701.

const state = deepFreeze( {
userPermissions: {},
} );
expect( canUser( state, 'create', 'media' ) ).toBe( true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's surprising that we set true as a default value. What if REST API call takes 5 seconds and UI would be rendered before that happens?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, true as the default seems wrong to me. I'd rather UI update after API requests and have extra settings added than them removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed canUser() to return false by default in 62bcb19.

};
} ),
withDispatch( ( dispatch, { onToggle = noop } ) => {
withDispatch( ( dispatch, { clientId, onToggle = noop }, { select } ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select - that's interesting to see it have much broader application than I anticipated. I hope there are no drawbacks of that approach :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that in this case it lets us avoid passing unnecessary props to the component.

! isReusableBlock( blocks[ 0 ] ) ||
! getReusableBlock( blocks[ 0 ].attributes.ref )
// Show 'Convert to Regular Block' when selected block is a reusable block
isVisible: isReusable || (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better to move isVisible outside of the object - the way it was before. It makes return value harder follow at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 1c3f722

path,
// Ideally this would always be an OPTIONS request, but unfortunately there's
// a bug in the REST API which causes the Allow header to not be sent on
// OPTIONS requests to /posts/:id routes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the case, it should be fixed /cc @danielbachhuber @kadamwhite

* @param {string} resource REST resource to check, e.g. 'media' or 'posts'.
* @param {?string} id ID of the rest resource to check.
*
* @return {boolean} Whether or not the user can perform the action.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the information about the default behavior when it is not found in the store?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Done as part of 62bcb19.

@gziolo gziolo added the REST API Interaction Related to REST API label Dec 14, 2018
@gziolo
Copy link
Member

gziolo commented Dec 14, 2018

The existing hasUploadPermissions() selector is changed to use canUser( 'create', 'media' ) under the hood.

@imath - I think you implemented the original version of this behavior when working on media upload permissions. Can you double check if the generalized version is still correct?

@danielbachhuber or @kadamwhite - it would be nice to get your review on the bits that interact with REST API.

@noisysocks - nice work on the PR. I personally think it is quite close to be considered ready. My comments are mostly nitpicks. The only important thing from my perspective is to ensure that canUser has the best strategy for returning the default value. It feels like assuming there is no permission is safer.

@danielbachhuber
Copy link
Member

Improves the UX of creating, editing, and deleting a reusable block when logged in as an author or contributor by disabling the Add to Reusable Blocks, Edit, and Remove from Reusable Blocks buttons when necessary.

Does this change the UI at all? If so, can you include screenshots and/or GIFs in the description?

@danielbachhuber
Copy link
Member

Also:

The only important thing from my perspective is to ensure that canUser has the best strategy for returning the default value.

At first glance, I'm not sure how well this canUser abstraction is going to work in practice. WordPress' roles and capabilities system is quite nuanced, and abstractions on top of it can be problematic. Just want to flag that.

@imath
Copy link
Contributor

imath commented Dec 15, 2018

@gziolo Hi,

I've just tested the PR. I confirm that being logged in as an Administrator I'm still able to upload files and that being logged in as a contributor I can only use external URLs to add media. So it seems to be working as expected for this part 👍

@noisysocks FYI I also tested the Reusable blocks UI changes being logged in as a contributor:

  • I confirm I cannot create new reusable block.
  • I can use another user reusable block but i cannot edit it.
  • I can access to the /wp-admin/edit.php?post_type=wp_block manage screen but can only export the blocks created by others as json.

@talldan talldan dismissed their stale review December 17, 2018 03:44

Feedback addressed

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good—I like the canUser selector API, but I think there's some code cleanup here to do and I have two concerns:

  1. why is true the default response for canUser()?
  2. Why are we ignoring the API request throwing an error?


* state: Data state.
* action: Action to check. One of: 'create', 'read', 'update',
'delete'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indentation is kinda odd; I'm not sure why this is indented where it is...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indentation is kinda odd; I'm not sure why this is indented where it is...

Likely because it's unprocessed from the space-aligned JSDoc. Could probably be improved with some additional processing (lines.map( trim ).join( ' ' )?). It has no difference in the Markdown preview though, since the whitespace is collapsed.

@@ -159,6 +162,7 @@ export default compose( [
isFetching: isFetchingReusableBlock( ref ),
isSaving: isSavingReusableBlock( ref ),
block: reusableBlock ? getBlock( reusableBlock.clientId ) : null,
canUpdateBlock: !! reusableBlock && ! reusableBlock.isTemporary && canUser( 'update', 'blocks', ref ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you're checking !! reusableBlock rather than just using reusableBlock? Just curious!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you're checking !! reusableBlock rather than just using reusableBlock? Just curious!

If a value is falsey in this scenario, the specific value is returned in the left-hand of the condition.

If reusableBlock was undefined for example, without the !!, canUpdateBlock would be passed as undefined, despite its name indicating that it would be a boolean. As written, it satisfies an implicit contract of providing a boolean value. Practically speaking this would rarely have much an impact, except in a scenario where someone expected it to be the particular boolean form (=== false).

@@ -116,5 +155,7 @@ export function* hasUploadPermissions() {
allowHeader = get( response, [ 'headers', 'Allow' ], '' );
}

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd need template strings for the first part:

Suggested change
const key = compact( [ action, resource, id ] ).join( '/' );
const path = `/wp/v2/${ resource }` + ( id ? `/${ id }` : '' );

I think it's a bit easier to sort out though, yeah.

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );
const isAllowed = includes( allowHeader, method );
yield receiveUserPermissions( key, isAllowed );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least could we add the ticket URL in the comment so it can be tracked/checked on?

parse: false,
} );
} catch ( error ) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😟 This seems like it should throw. Why are we catching an ignoring the error without doing anything with it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error here refers to the API error that resulted from our OPTIONS request. Doing nothing is correct in this case as we want to leave the store as is. I've added a comment to explain this in e0a7269.

const state = deepFreeze( {
userPermissions: {},
} );
expect( canUser( state, 'create', 'media' ) ).toBe( true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, true as the default seems wrong to me. I'd rather UI update after API requests and have extra settings added than them removed.

blocks.length === 1 &&
blocks[ 0 ] &&
isReusableBlock( blocks[ 0 ] ) &&
!! getReusableBlock( blocks[ 0 ].attributes.ref )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use !! here? I just always find it weird to see 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to ensure that typeof isReusable === 'boolean'.


every( blocks, ( block ) => (
// Guard against the case where a regular block has *just* been converted
!! block &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, any reason for the !!?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to ensure that typeof isVisible === 'boolean'.

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from 9ec72e8 to cf2d701 Compare December 24, 2018 04:59
@danielbachhuber danielbachhuber removed their request for review December 26, 2018 17:04
@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from 15ff121 to 7b8d040 Compare January 23, 2019 05:02
@talldan
Copy link
Contributor

talldan commented Jan 23, 2019

IIRC this one also needed a change in core to preload that request.

https://github.com/WordPress/gutenberg/pull/12378/files#diff-620130744b6b73b822a3df609671d930R1089

@noisysocks noisysocks merged commit 33d2acb into master Jan 23, 2019
@noisysocks noisysocks deleted the fix/non-privileged-reusable-blocks-ux branch January 23, 2019 05:41
@noisysocks
Copy link
Member Author

noisysocks commented Jan 23, 2019

Getting this in early so that there's time for it to bake in 5.0.

IIRC this one also needed a change in core to preload that request.

Thanks for the reminder. I wonder what the best way to do this is... Do we check how the plugin PHP files differ when updating Core's script dependencies? (e.g. git diff v4.9 v5.0 **.php) If so, we'll catch this change. If not, perhaps opening a ticket is the safest bet, though I'm not actually sure when we plan on updating Core to use Gutenberg 5.0 scripts.

@talldan
Copy link
Contributor

talldan commented Jan 23, 2019

@noisysocks Not sure. This one was done as a trac ticket, so maybe that's the best option:
https://core.trac.wordpress.org/changeset/43833

@@ -24,6 +24,7 @@
"@babel/runtime": "^7.0.0",
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/data": "file:../data",
"@wordpress/deprecated": "file:../deprecated",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should have been a corresponding change to the root package-lock.json. I'm not sure how this passed the build task which checks for local uncommitted changes, but it shouldn't have.

Copy link
Member Author

@noisysocks noisysocks Jan 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh! Looks like check-local-changes doesn't pick this up because precheck-local-changes doesn't run npm install. CI doesn't pick it up because it uses npm ci.

Will fix this up in a seperate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 6, 2019

A test that I made:
Contributor made a new post. Submitted for Review.
Logged in as admin. Went to the newly created contributor post and took it over. Made a reusable block. Clicked to save as pending.
Logged in as contributor. Went to post. Noticed that contributor could delete the reusable block.

@talldan
Copy link
Contributor

talldan commented Feb 6, 2019

@paaljoachim What version are you testing against? This has only just been released in the plugin today afaik.

I couldn't reproduce testing against master.

@paaljoachim
Copy link
Contributor

Hey Daniel @talldan

Lets just call it an error on my end. I made the following comment:
https://make.wordpress.org/test/2019/02/05/call-for-testing-gutenberg-5-0/#comment-1170

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
Improves the UX of creating, editing, and deleting a reusable block when
logged in as an author or contributor by disabling the _Add to Reusable
Blocks_, _Edit_, and _Remove from Reusable Blocks_ buttons when
necessary.

This is accomplished under the hood by introducing the `canUser()`
selector to `core-data` which allows callers to query whether the REST
API supports performing a given action on a given resource, e.g. one can
query whether the logged in user can create posts by running
`wp.data.select( 'core' ).canUser( 'create', 'posts' )`.

The existing `hasUploadPermissions()` selector is changed to use
`canUser( 'create', 'media' )` under the hood.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
Improves the UX of creating, editing, and deleting a reusable block when
logged in as an author or contributor by disabling the _Add to Reusable
Blocks_, _Edit_, and _Remove from Reusable Blocks_ buttons when
necessary.

This is accomplished under the hood by introducing the `canUser()`
selector to `core-data` which allows callers to query whether the REST
API supports performing a given action on a given resource, e.g. one can
query whether the logged in user can create posts by running
`wp.data.select( 'core' ).canUser( 'create', 'posts' )`.

The existing `hasUploadPermissions()` selector is changed to use
`canUser( 'create', 'media' )` under the hood.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.